-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove deprecated method #8810
base: master
Are you sure you want to change the base?
Remove deprecated method #8810
Conversation
499e022
to
9bc74b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MGaetan89 IIRC, many deprecated methods can not be deleted directly because widely used. Sorry about it.
No worry. I saw that there are many deprecated methods in the projects, but only removed these because there is a removal version in te deprecation message. |
Thanks for doing this, I will take a closer look at this tomorrow. |
9bc74b6
to
fa1a0da
Compare
Looks like there are some tests at Google that are using ServiceController.withIntent(Intent) to deliver new intents to an existing Service object. This appears to be a valid testing use case and there does not appear to be an alternative API for this. This has also come up here as well: #3277 (comment). Anyway, long-story short, we probably need to keep |
Looks like this is used by Litho: We should check if LithoTestRunner can be updated to remove that. Other than these two, I am not seeing any compatibility issues. |
@hoisie I recommend to clean-up remove time in comments instead of removing them. It's very hard to find all potential usages now. If we really want to remove them, we can remove them in multiple Robolectric versions but from deprecated first. Maybe two years ago, I still want to clean-up these methods, but I found there were some important methods that marked by deprecated were used by many projects we can't provide alternative stable methods for them. I know it's great if we can let developers use standard APIs of Robolectric, instead of some helper methods like configuration APIs for purpose usage. |
After reviewing code again, if we can confirm that we can use standard APIs to retrieve Display's metrics and real metrics, we can remove these two deprecated APIs first instead of full list that the current PR wants to remove. |
I've tried to do just that (removing If that's fine by you, I can update this PR as explained. Otherwise, if the other methods need to be kept, I'd say that we can close this PR. |
I am OK with having a PR that removes all of the methods on ShadowDisplay |
These methods have been deprecated for years right? In general I am OK with deleting things even if teams have to fix small things when they upgrade. Robolectric has a MASSIVE public API surface and it's still growing. I think it's OK to be a little aggressive about removing stuff that has been deprecated. People can always use previous versions for as long as they need. |
@MGaetan89 If you would like, you can send a new PR to delete deprecated methods for |
I agree with you that developers only need to modify small patch for a new version. But if deleted APIs are important configuration related APIs, it is always very easily. I have seen some people customize the Robolectric a lot based on these deprecated internal APIs. The adaption might be easier if developers can replace it with other public APIs, but some APIs without replacements. If we delete the deprecated APIs, developers who use them need to delete related tests as they can't use other public APIs to make related tests work again. |
fa1a0da
to
513aff7
Compare
I've updated the description of the PR to includes links to usages of each deprecated method. |
robolectric/src/main/java/org/robolectric/RobolectricTestRunner.java
Outdated
Show resolved
Hide resolved
513aff7
to
71832ba
Compare
@MGaetan89 Maybe we can make a new PR to delete withIntent only? If we need to provide an alternative method, we can discuss in that PR. |
Sure, I'll do that 👍🏻 |
These methods were supposed to be removed in older versions of Robolectric
71832ba
to
31457d5
Compare
These methods were supposed to be removed in older versions of Robolectric:
ConfigMerger
RobolectricTestRunner.buildGlobalConfig()
ServiceController.withIntent(Intent)
Note: I've marked
RobolectricTestRunner.CONFIG_PROPERTIES
as deprecated, because it is no longer used in the project, but it's stillpublic
. Let me know if you want to proceed otherwise.